Skip to content

fix: Enforce a 24-hour maximum session duration#494

Open
turnipdabeets wants to merge 15 commits intomainfrom
fix/use-date-provider-in-session-manager
Open

fix: Enforce a 24-hour maximum session duration#494
turnipdabeets wants to merge 15 commits intomainfrom
fix/use-date-provider-in-session-manager

Conversation

@turnipdabeets
Copy link
Copy Markdown
Contributor

@turnipdabeets turnipdabeets commented Apr 21, 2026

💡 Motivation and Context

Enforce a 24-hour maximum session duration. When a session exceeds 24 hours, it is automatically rotated to a new one. This prevents excessively long sessions from accumulating unbounded data.

Continues @marandaneto's work from #470 (auto-closed by stale-bot; branch was rebased + force-pushed so GitHub wouldn't allow reopening). Adds one follow-up commit on top to close a replay gap.

Closes #220 for Android

💚 How did you test it?

  • Unit tests for rotateSession(), getSessionStartedAt(), and sessionStartedAt lifecycle in PostHogSessionManagerTest.
  • Integration tests in PostHogLifecycleObserverIntegrationTest verifying:
    • Session is rotated when it exceeds 24 hours (foreground path)
    • Session is not rotated when under 24 hours
    • Session rotation is skipped for React Native
    • New: Replay is restarted after 24h rotation in background when replay was active
    • New: Replay is not restarted after 24h rotation when replay was inactive
  • Used PostHogDateProvider for time mocking.
  • make checkFormat passes.
Manual test on emulator (Pixel 9 / API 17)

Setup: temporarily flip both constants in PostHogSessionManager.kt:
private const val SESSION_MAX_DURATION = (1000L * 60 * 2) // 2 min for testing
private const val SESSION_INACTIVITY_DURATION = (1000L * 30) // 30s for testing

Build + install: ./gradlew :posthog-samples:posthog-android-sample:installDebug

Watch logs (Terminal A): adb logcat -c && adb logcat PostHog:D '*:S'

Drive scenarios (Terminal B):

A. Foreground 30s idle → rotate via getter

adb shell am force-stop com.posthog.android.sample
adb shell am start -n com.posthog.android.sample/.NormalActivity
adb shell input tap 500 1000 # event 1 → SID_A1
sleep 35 # >30s idle in foreground
adb shell input tap 500 1000 # event 2 → SID_A2
Expected: SID_A2 ≠ SID_A1. [Session Replay] Session changed. Re-initializing recording for new session. log fires.

B. Active foreground does NOT rotate

adb shell am force-stop com.posthog.android.sample
adb shell am start -n com.posthog.android.sample/.NormalActivity
adb shell input tap 500 1000 # SID_B1
for i in 1 2 3; do sleep 20; adb shell input tap 500 1000; done
Expected: all events ship with same $session_id. touchSession() on each tap keeps the activity timestamp fresh.

C. Background → 35s → foreground rotates with replay re-init

adb shell am force-stop com.posthog.android.sample
adb shell am start -n com.posthog.android.sample/.NormalActivity
adb shell input tap 500 1000 # SID_C1
adb shell input keyevent KEYCODE_HOME
sleep 35
adb shell am start -n com.posthog.android.sample/.NormalActivity
adb shell input tap 500 1000 # SID_C2
Expected: SID_C2 ≠ SID_C1, and the first snapshot under SID_C2 contains rrweb type-4 (meta) + type-2 (full wireframe). Foregrounding triggers a redraw which emits the fresh
keyframes.

D. Brief background does NOT rotate

adb shell am force-stop com.posthog.android.sample
adb shell am start -n com.posthog.android.sample/.NormalActivity
adb shell input tap 500 1000 # SID_D1
adb shell input keyevent KEYCODE_HOME
sleep 10
adb shell am start -n com.posthog.android.sample/.NormalActivity
adb shell input tap 500 1000
Expected: same $session_id for all events. Under-threshold bg → no rotation.

E. 24h max via continuous foreground (regression check)

adb shell am force-stop com.posthog.android.sample
adb shell am start -n com.posthog.android.sample/.NormalActivity
adb shell input tap 500 1000
for i in 1 2 3 4 5 6 7 8; do sleep 20; adb shell input tap 500 1000; done
Expected: session id changes mid-stream after 2 min of activity (taps every 20s keep inactivity at bay). Fresh keyframes appear under the new session on the next view change.

After testing — revert constants:
private const val SESSION_MAX_DURATION = (1000L * 60 * 60 * 24) // 24 hours
private const val SESSION_INACTIVITY_DURATION = (1000L * 60 * 30) // 30 minutes

About the follow-up commit

When the 24h limit expires while the app is backgrounded, onStop correctly ends the session and stops replay. But the next onStart falls into the lastUpdatedSession == 0L branch, which only called startSession() — so replay stayed off under the new session, even though the foreground-rotation branch (else if at PostHogLifecycleObserverIntegration.kt:77) does auto-restart it. The new commit makes both paths symmetrical by tracking replayActiveBeforeRotation in onStop and resuming replay on the next onStart when the flag is set.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran pnpm changeset to generate a changeset file
  • Added the "release" label to the PR to indicate we're publishing new versions for the affected packages

marandaneto and others added 9 commits April 21, 2026 13:05
When the 24h session limit expires while the app is backgrounded,
onStop ends the session and stops replay. The subsequent onStart
was only calling startSession(), leaving replay disabled even
when session replay was active before the rotation. Mirror the
foreground-rotation branch by tracking whether replay was active
prior to rotation and restarting it under the new session.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 21, 2026

posthog-android Compliance Report

Date: 2026-04-22 14:09:06 UTC
Duration: 363ms

⚠️ Some Tests Failed

0/1 tests passed, 1 failed


Feature_Flags Tests

⚠️ 0/1 tests passed, 1 failed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id 224ms

Failures

request_payload.request_with_person_properties_device_id

404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'

@dustinbyrne
Copy link
Copy Markdown
Contributor

Slight behavioral drift vs. iOS:

In iOS when we retrieve the active session ID, it checks to see if it's still valid before returning the identifier. If it's not, it's immediately rotated. https://github.com/PostHog/posthog-ios/blob/c165ead50ecf66368685a8467fa625fc0960bace/PostHog/PostHogSessionManager.swift#L109-L114

This differs from the current implementation in that we only rotate the session on a fg/bg transition.

iOS calls this getter when building event properties, so it can happen during any event transmission: https://github.com/PostHog/posthog-ios/blob/c165ead50ecf66368685a8467fa625fc0960bace/PostHog/PostHogSDK.swift#L421-L423

There's a test that validates this, it's probably worth porting over: https://github.com/PostHog/posthog-ios/blob/c165ead50ecf66368685a8467fa625fc0960bace/PostHogTests/PostHogSessionManagerTest.swift#L282-L283

Mirrors the iOS pattern: getActiveSessionId() now checks expiry on
every read and rotates (foreground) or clears (background) when the
session has lived past SESSION_MAX_DURATION. Previously, rotation
only fired on lifecycle transitions, so a continuously foregrounded
app could ride a stale session id indefinitely.

Adds setAppInBackground() toggled by the lifecycle observer and a
setOnSessionIdChangedListener() registered in PostHog.setup() to
notify the session replay handler when the getter rotates silently.

Ports the iOS sessionRotatedAfterMaxSessionLength test plus sibling
cases for bg-clear, RN skip, under-24h no-op, and listener firing.
@marandaneto
Copy link
Copy Markdown
Member

@ioannisj implemented this on iOS so worth an extra pair of eyes (this also affects Flutter iirc)

Comment thread posthog/src/main/java/com/posthog/internal/PostHogSessionManager.kt Outdated
tempSessionId = if (sessionId != sessionIdNone) sessionId else null
} else {
val now = dateProvider?.currentTimeMillis() ?: System.currentTimeMillis()
val expired = sessionStartedAt > 0L && (sessionStartedAt + SESSION_MAX_DURATION) <= now
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On iOS we are also checking for 30min inactivity here. If I remember correctly, android detects this purely through background/foreground event. iOS has an activity timestamp tracking through touchSession() which basically rotates the session if the user has not been interacting with the app in 30mins (background or not). Would be good to close this gap as well between the two

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Added touchSession() mirroring iOS, wired into lifecycle onStart, replay touch listener, and capture() (the last one is an Android-specific deviation since we don't have UIEvent swizzling). Verified manually with shortened timers: foreground idle rotates, active foreground doesn't, brief bg preserves session.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wired into lifecycle onStart, replay touch listener, and capture()

Awesome. For replay touch listener, does this mean that we'll get a different behavior on session rotation when replay is not enabled? Session id affects session-metrics calcs so it would be good to decouple.

queue.start()

PostHogSessionManager.setOnSessionIdChangedListener {
sessionReplayHandler?.onSessionIdChanged()
Copy link
Copy Markdown
Contributor

@ioannisj ioannisj Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any changes on resetting state for session replay inside onSessionIdChanged() (unless it's already properly handled), since this is now a new recording and we'll need metadata/full snapshot. Can you please test how replay behaves with session rotations (maybe set max session length to a couple of mins locally)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated! onSessionIdChanged() now does stop() + start(resumeCurrent = false) on rotation (which clears per-view snapshot state so the next onDraw emits fresh type-4 + type-2), or stop() on bg-clear. Verified end-to-end with shortened constants: bg→fg rotation produces clean keyframes immediately and mid-foreground rotation produces keyframes on the next visible redraw.

Mirrors the iOS pattern: when properties already contains $session_id
(e.g., session replay attaches it at frame-build time), use it directly
instead of calling PostHogSessionManager.getActiveSessionId(). The
getter can silently rotate the manager's session, but the caller's
value wins downstream via putAll — so the rotation would be wasted.
…close

Adds the missing PostHog-level test verifying that a caller-provided
$session_id wins over the session manager's value (the change in
165e5f2 had no integration test). Also clears the session listener
in PostHog.close() to avoid leaking the PostHog instance via the
process-singleton PostHogSessionManager, and simplifies a
?.also { it.invoke() } down to ?.invoke().
Addresses two points from @ioannisj's review:

- setSessionId now stamps sessionStartedAt so an externally-set
  session participates in the 24h expiry check. Without it,
  sessionStartedAt stayed 0 and the session would never expire.

- PostHogReplayIntegration.onSessionIdChanged now restarts the
  recording when the session rotates silently (e.g., 24h getter
  rotation), so the new session emits fresh meta + full wireframe
  events. Previously the new session received only incremental
  events, leaving the replay viewer with no baseline to render.
  If the session was cleared (background expiry), recording stops
  outright since snapshots without $session_id are dropped anyway.
The replay restart from onSessionIdChanged calls start(resumeCurrent=false)
which iterates a non-thread-safe WeakHashMap. The getter listener can fire
from any thread that calls capture(), so post both stop and start to the
main handler.

Also adds a PostHogTest that verifies the listener actually wires through:
forces an expired session via setSessionId + a backdated date provider,
calls getSessionId(), and asserts the fake replay handler's
onSessionIdChangedCalled flag flips. Drops a weak assertion from the
caller-provided session_id test that was tautologically true.
Closes the parity gap with iOS PostHogSessionManager that ioannisj
flagged. The Android session manager only rotated on background/
foreground transitions and on the 24h max-duration check; iOS also
rotates after 30min of no user activity, regardless of fg/bg state.

Manager changes:
- New sessionActivityTimestamp field tracking last activity.
- New touchSession() method mirroring iOS: rotates if idle past
  SESSION_INACTIVITY_DURATION (30min), else refreshes timestamp.
  No-op when backgrounded so background events don't keep a dead
  session alive.
- getActiveSessionId() also checks inactivity (in iOS order:
  inactivity first, then 24h max).
- Extracted isIdle/isMaxExpired/rotateLocked/clearLocked helpers
  to deduplicate the three places that compute expiry, and pulled
  the React Native check inside the lock to remove a TOCTOU race.

Wiring (call sites for touchSession):
- PostHog.capture(): touch at the start so any captured event
  counts as activity. iOS achieves this via UIEvent swizzling;
  Android lacks the equivalent so capture() is the safe fallback.
- PostHogLifecycleObserverIntegration.onStart: touch after
  setAppInBackground(false) (mirror iOS lifecycle hook).
- PostHogReplayIntegration.onTouchEventListener: touch on every
  intercepted touch (closest Android equivalent to iOS UIEvents).

The lifecycle observer's existing 30min Timer is now overlapping
with the manager's bg-clear path; kept as defense-in-depth for
backgrounded apps that don't fire any events.

Tests: added 7 unit tests for touchSession behavior, foreground
inactivity rotation, background inactivity clear, and the
no-op-while-backgrounded guarantee. Added a PostHog-level test
verifying that capture() rotates an idle session via touchSession
before reading the session id. Updated the existing
"under 24 hours" test to keep the session active with periodic
touches so the new inactivity check doesn't break it.
@turnipdabeets turnipdabeets requested a review from ioannisj April 22, 2026 20:42
@turnipdabeets
Copy link
Copy Markdown
Contributor Author

@ioannisj when you get a moment, mind giving this another review?

synchronized(sessionLock) {
val now = dateProvider?.currentTimeMillis() ?: System.currentTimeMillis()
this.sessionId = sessionId
// Stamp the start so an externally-set session participates in the 24h
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably only need to do this if this.sessionId != sessionId (before assigning right above)

tempSessionId = if (sessionId != sessionIdNone) sessionId else null
} else {
val now = dateProvider?.currentTimeMillis() ?: System.currentTimeMillis()
val expired = sessionStartedAt > 0L && (sessionStartedAt + SESSION_MAX_DURATION) <= now
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wired into lifecycle onStart, replay touch listener, and capture()

Awesome. For replay touch listener, does this mean that we'll get a different behavior on session rotation when replay is not enabled? Session id affects session-metrics calcs so it would be good to decouple.

override fun onSessionIdChanged() {
val postHog = this.postHog ?: return

val currentSessionId = postHog.getSessionId()?.toString()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can lead to infinite recursion since getSessionId() is mutating, and we are reacting to a new session id change? Not sure how much of an edge case this can be, if at all, so happy for a push back here. I feel a read-only operation on PostHogSessionManager would be safer though

config.logger.log("[Session Replay] Session changed. Stopping until trigger is matched.")
stop()
}
} else if (isSessionReplayActive) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should react even if session replay is not active. Replay may be enabled in config but Session A may not have been sampled and not started. Now that we rotate to session B, sampling may return true and session replay should start?

config.logger.log("[Session Replay] Session changed. Re-initializing recording for new session.")
mainHandler.handler.post {
stop()
start(resumeCurrent = false)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the comment above, this will skip sampling check and start the session anyway? Maybe we can route through PostHog.startSessionReplay() which checks sampling?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enforce max session duration

4 participants